Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multiple diff report formats #152

Merged
merged 12 commits into from
Sep 26, 2023

Conversation

Leftwitch
Copy link

@Leftwitch Leftwitch commented Sep 8, 2023

Description

This PR aims to implement the feature described in issue #137, which allows the creation of a Markdown report containing the differences between the two compared APIs.

Currently, it displays the API differences in the Markdown report but does not include any semver warnings or errors. This omission was intentional because it wasn't clear if it was a requirement. However, we can add this functionality if needed.

Additionally, this PR lays the foundation for future report formats. It introduces an abstraction layer to the diff command, which now supports the following report formats:

When you run the diff command as you normally would, the output remains the same as before, with no changes. However, you now have the option to specify the report format and file path using the following command-line options:

  • report-format
  • report-file-path

For more details on how to use these options, please refer to the updated Readme or use the -h option when invoking the command.

Type of Change

  • [ X] 🚀 New feature (non-breaking change)
  • 🛠️ Bug fix (non-breaking change)
  • ⚠️ Breaking change (feature or bug fix which breaks existing behaviors/APIs)
  • 🏗️ Code refactor
  • ⚙️ Build configuration change
  • [X ] 📝 Documentation
  • 🧹 Chore / Housekeeping

@Leftwitch Leftwitch marked this pull request as ready for review September 8, 2023 14:22
@devmil
Copy link
Member

devmil commented Sep 8, 2023

Hi. Thank you for your contribution. I will have a look at your changes as soon as I can

@Leftwitch
Copy link
Author

Great, I noticed that a test didn't pass. I'll address that issue. However, I hope it won't obstruct your review of the code and my approach.

Hopefully my contribution is useful for you :)

@devmil
Copy link
Member

devmil commented Sep 8, 2023

Great, I noticed that a test didn't pass. I'll address that issue. However, I hope it won't obstruct your review of the code and my approach.

Hopefully my contribution is useful for you :)

The failed test seems to be a timeout issues. Maybe you had bad luck with the build runner. Maybe we need to increase the timeout

@Leftwitch
Copy link
Author

Ah! Just saw that aswell, okay in that case I'm going to leave it as-is, and make changes after your feedback :)

Copy link
Member

@devmil devmil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some remarks / questions.
I think we should add some (basic) unit tests for the reporter just to make sure that they work in general

_optionReportFormat,
help: 'Which output format should be used',
defaultsTo: 'cli',
allowed: ['cli', 'markdown', 'json'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use an enum instead of strings for that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the Documentation of the args package and couldn't find anything like enum options, of course I could use something like .toString() and parse it later into the enum, but is that the direction you were aiming for or your meant something else?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right: Enums are not directly supported by the args package.
You can have a look at the DependencyCheckMode arg for mapping between enum and args:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Locally I changed that and it seems to work, gonna push it when my tests are done aswell

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the already existing tests, would you prefer extending the diff_command_test to check if the command handles the arguments accordingly? or what kind of Tests you had in mind? I was also looking into the unit test, but testing just the reporter class won't do much since it's just an abstract class which does nothing by its own.

So some feedback / opinion would be appreciated so I know what exactly to implement :)

Wish yall a nice Friday :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah dang it, I didn't get a Notification for that one, weird, Anyways the Tests look good, I think I got the gist now of what you meant ^^

And also I agree, when we go too much Into detail the tests might become to flaky

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: Since the json format also generates a file we could add a test for that case aswell I think

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a Test-Case for the JSON-Report, it also attempts to parse the JSON and check for some essential properties

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Leftwitch just ping me if you are done

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the Json test, if you don't have anything to add I'd say the PR is done so far :)

This is a breaking change because it is possible that a user of the library is using the interface. If the interface is removed, the user will get a compile error.

### <a name="CI02" />An interface is added (CI02)
### CI02
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a special reason for removal of the anchors?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When trying using the anchors in the URL it didn't work for me atleast, so I decided to nest them differently so I can link from the Markdown report to the correct section, however if you know another solution to make the "old" one work we can surely use that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense. 👍

@Leftwitch
Copy link
Author

I added some remarks / questions. I think we should add some (basic) unit tests for the reporter just to make sure that they work in general

Sounds like a good Idea, I will come up with some Basics tests and push a new commit to the branch, while doing so I'm also rebasing on main since we got a conflict ^^

@devmil
Copy link
Member

devmil commented Sep 26, 2023

@Leftwitch feel free to press the merge button 👍
Thanks again for your contribution!

@Leftwitch
Copy link
Author

@Leftwitch feel free to press the merge button 👍
Thanks again for your contribution!

I'm glad I could help :)!
Sure more to come!

I sadly can't merge the pill request I would need write access for that ^^ Just go ahead and you merge it :)

@devmil devmil merged commit b4c1d34 into bmw-tech:main Sep 26, 2023
11 checks passed
@Leftwitch Leftwitch deleted the feat/markdown-report branch September 26, 2023 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants